Skip to content

nplb: use SIGUSR2 in KillSendsSignalToSelf - #11335

Merged
sacuff merged 1 commit into
youtube:mainfrom
rogerzanoni:aosp-nplb-sigusr2
Jul 28, 2026
Merged

nplb: use SIGUSR2 in KillSendsSignalToSelf#11335
sacuff merged 1 commit into
youtube:mainfrom
rogerzanoni:aosp-nplb-sigusr2

Conversation

@rogerzanoni

Copy link
Copy Markdown
Collaborator

SIGUSR1 is intercepted by the Android runtime and never reaches the handler installed by the test. Switch the test to SIGUSR2.

Bug: 532068409

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the KillSendsSignalToSelf test in starboard/nplb/posix_compliance/posix_signal_test.cc to use SIGUSR2 instead of SIGUSR1 for signal verification. There are no review comments, and I have no feedback to provide.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Gemini Suggested Commit Message


starboard: Use SIGUSR2 in KillSendsSignalToSelf

The KillSendsSignalToSelf test currently fails on Android because
SIGUSR1 is intercepted by the Android runtime and never reaches the
handler installed by the test.

Switch to SIGUSR2 to ensure the signal is correctly delivered and
processed during the test.

Bug: 532068409

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

@sacuff

sacuff commented Jul 23, 2026

Copy link
Copy Markdown
Member

Is the behavior of SIGUSR1 documented anywhere by AOSP? I was able to find https://android.googlesource.com/platform/bionic/+/master/tests/signal_test.cpp#398 which leads me to believe it just blocks SIGKILL and SIGSTOP, in which case can we try unblocking them instead of switching to SIGUSR2? Ex:

ASSERT_EQ(sigprocmask(SIG_UNBLOCK, &unblock_mask, &original_mask_), 0);

@rogerzanoni

Copy link
Copy Markdown
Collaborator Author

@sacuff sorry, my change was poorly documented

I did the change based on ART's SignalCatcher that sets up handlers for SIGUSR1: https://android.googlesource.com/platform/art/+/refs/heads/main/runtime/signal_catcher.cc#207

and the runtime errors that show that it's being intercepted by it:

07-27 15:43:10.074 30939 30973 I starboard: [ RUN      ] PosixSignalTest.KillSendsSignalToSelf
07-27 15:43:10.074 30939 30950 I dev.cobalt.coa: Thread[5,tid=30950,WaitingInMainSignalCatcherLoop,Thread*=0xf1052800,peer=0x13780110,"Signal Catcher"]: reacting to signal 10

posix_signal_test already tries to unblock SIGUSR1, so I tried adding SIGKILL and SIGSTOP, but the signal is still being intercepted by the ART's signal catcher.

I think one possible solution is to do something similat to the fix of #11340 and make the signal thread-directed in a posix_emu kill() wrapper, so it ART won't have the chance to intercept it, wdty? AFAIU it's a very similar issue

@sacuff

sacuff commented Jul 28, 2026

Copy link
Copy Markdown
Member

@jellefoks I see SIGUSR1 (and 2) are used for lifecycle stuff. Do you have an opinion on how AOSP should handle the interception here?

@sacuff
sacuff requested a review from jellefoks July 28, 2026 00:25
@rogerzanoni

Copy link
Copy Markdown
Collaborator Author

I created this as a proposal: #11562

@sacuff sacuff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed the signal usage more with Jelle. SIGUSR1 is used on Linux for lifecycle events, but so is SIGUSR2 so this change should be fine without needing to change of the signal handling on Android.

Not relevant to NPLB: on AOSP we will want to make sure to translate lifecycle events to intents instead of going through the signal route.

@sacuff
sacuff enabled auto-merge (squash) July 28, 2026 21:17
SIGUSR1 is intercepted by the Android runtime and never reaches the
handler installed by the test. Switch the test to SIGUSR2.

Bug: 532068409
@sacuff
sacuff force-pushed the aosp-nplb-sigusr2 branch from 8b321ef to a7e54a2 Compare July 28, 2026 21:18
@sacuff
sacuff merged commit b6c1a9d into youtube:main Jul 28, 2026
632 of 636 checks passed
@github-actions github-actions Bot added the cp-27.lts Cherry Pick to the 27.lts branch label Jul 28, 2026
cobalt-github-releaser-bot pushed a commit that referenced this pull request Jul 28, 2026
Refer to original PR: #11335

SIGUSR1 is intercepted by the Android runtime and never reaches the
handler installed by the test. Switch the test to SIGUSR2.

Bug: 532068409

(cherry picked from commit b6c1a9d)
briantting pushed a commit that referenced this pull request Jul 28, 2026
Refer to original PR: #11335

SIGUSR1 is intercepted by the Android runtime and never reaches the
handler installed by the test. Switch the test to SIGUSR2.

Bug: 532068409

(cherry picked from commit b6c1a9d)
briantting pushed a commit that referenced this pull request Jul 28, 2026
Refer to original PR: #11335

SIGUSR1 is intercepted by the Android runtime and never reaches the
handler installed by the test. Switch the test to SIGUSR2.

Bug: 532068409

(cherry picked from commit b6c1a9d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cp-27.lts Cherry Pick to the 27.lts branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants